-
Notifications
You must be signed in to change notification settings - Fork 562
feat(stdlib): Add source information for slow outgoing HTTP requests #4902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
span.finish() | ||
|
||
with capture_internal_exceptions(): | ||
add_http_request_source(span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I placed add_http_request_source(span)
after the span is finished because you need the end timestamp to determine the delay in receiving a response to the HTTP request.
It's done analogously in asyncpg
and sqlalchemy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: It'd be a concern in (P)OTel as finished OTel spans can't be modified, but Sentry spans can so all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! See one optional suggestion.
span.finish() | ||
|
||
with capture_internal_exceptions(): | ||
add_http_request_source(span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun fact: It'd be a concern in (P)OTel as finished OTel spans can't be modified, but Sentry spans can so all good.
error_sampler=None, # type: Optional[Callable[[Event, Hint], Union[float, bool]]] | ||
enable_db_query_source=True, # type: bool | ||
db_query_source_threshold_ms=100, # type: int | ||
enable_http_request_source=True, # type: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally ok with making this True
by default from the get go since it's basically a copy of the existing DB query source feature, but we could also do a soft launch with the default set to False
, dogfood in Sentry, and then set this to True
in the next release. I'll leave it up to you.
Description
Add code source as described in getsentry/sentry-docs#15161.
Factors out functionality from SQL query source and tests that it works in the HTTP request setting.
Issues
Closes #4881
Reminders
tox -e linters
.feat:
,fix:
,ref:
,meta:
)